Skip to content

feat(design-system): add DsSplitButton component [AR-54024]#347

Merged
iromanchuk-dn merged 17 commits intodrivenets:mainfrom
iromanchuk-dn:AR-54024-design-system-split-button
Apr 15, 2026
Merged

feat(design-system): add DsSplitButton component [AR-54024]#347
iromanchuk-dn merged 17 commits intodrivenets:mainfrom
iromanchuk-dn:AR-54024-design-system-split-button

Conversation

Comment thread packages/design-system/src/components/ds-split-button/index.ts Outdated
@vpolessky-dn
Copy link
Copy Markdown
Collaborator

missing browser test fro the button

Comment thread packages/design-system/src/components/ds-split-button/ds-split-button.types.ts Outdated
$divider-z-index: $highlighted-z-index + 1;
$divider-width: 1px;
$border-width: 1px;
$button-transition-duration: 0.15s;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transition standard is .2s

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it aligned with DsButtonV3, it's important to have the same duration, because $button-transition-duration affects button right border.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe add a comment about this?
Or even better, let's try to find a way to share it and have a single source of truth
I'll just note that I'm afraid that importing it as a SCSS variable will import & bundle the whole button SCSS file, so need to be careful with that

vpolessky-dn
vpolessky-dn previously approved these changes Apr 9, 2026
'aria-label': 'Refresh',
onClick,
},
select: { ...defaultSelect, onValueChange: vi.fn() } as DsSelectProps,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you don't need any of the as DsSelectProps

value: '30',
onValueChange: vi.fn(),
multiple: false,
} as const satisfies Partial<DsSelectProps>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be simplified to:

Suggested change
} as const satisfies Partial<DsSelectProps>;
} satisfies DsSelectProps;

'aria-label': 'Refresh',
onClick,
},
select: { ...defaultSelect, onValueChange: vi.fn() } as DsSelectProps,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're overriding an existing prop in all tests for no reason. This can be simplified in all cases:

Suggested change
select: { ...defaultSelect, onValueChange: vi.fn() } as DsSelectProps,
select: defaultSelect,

$divider-z-index: $highlighted-z-index + 1;
$divider-width: 1px;
$border-width: 1px;
$button-transition-duration: 0.15s;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe add a comment about this?
Or even better, let's try to find a way to share it and have a single source of truth
I'll just note that I'm afraid that importing it as a SCSS variable will import & bundle the whole button SCSS file, so need to be careful with that

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there are some padding-top issues with the small size (maybe bug in the select component itself?):
image

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a known bug in the DsSelect component.
https://drivenets.atlassian.net/browse/AR-52305

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this component closed to a specific use case / composition?

That's not the what happens in MUI or Chakra

(also, maybe we could reconsider the naming here, according to the above examples)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have discussed this idea in the team. Designers do not have a plan to combine other components in a similar manner. Also it would be extremely hard to handle all possible scenarios and support them.

Comment on lines +16 to +18
const { className: buttonClassName, disabled: buttonDisabled, ...buttonRest } = slotProps.button;

const { className: selectClassName, disabled: selectDisabled, ...selectRest } = slotProps.select;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, non-blocker

maybe this is nicer? 😅

Suggested change
const { className: buttonClassName, disabled: buttonDisabled, ...buttonRest } = slotProps.button;
const { className: selectClassName, disabled: selectDisabled, ...selectRest } = slotProps.select;
const { className: buttonClassName, disabled: buttonDisabled, ...buttonProps } = slotProps.button;
const { className: selectClassName, disabled: selectDisabled, ...selectProps } = slotProps.select;

Comment on lines +10 to +11
type DistributiveOmit<T, K extends PropertyKey> = T extends unknown ? Omit<T, K> : never;
type SelectSlotProps = DistributiveOmit<DsSelectProps, 'size'>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

maybe at some point we'll have a type-utils.ts file
I guess we should keep this in mind once we'll add more utils like this

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I was thinking about the same. It looks like an utility we may use in multiple components. I think I will do it now, someone needs to start it.

Comment on lines +4 to +6
export const getSelectSize = (size: SplitButtonSize): SelectSize => {
return size === 'medium' ? 'default' : 'small';
};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO having a whole file for this is overkill
I would put this in the component itself as a variable 🤷

@iromanchuk-dn iromanchuk-dn requested a review from StyleShit April 15, 2026 11:38
mmurawski-dn
mmurawski-dn previously approved these changes Apr 15, 2026
StyleShit
StyleShit previously approved these changes Apr 15, 2026
@iromanchuk-dn iromanchuk-dn dismissed stale reviews from StyleShit and mmurawski-dn via 061d838 April 15, 2026 13:04
@iromanchuk-dn iromanchuk-dn merged commit 43d78e2 into drivenets:main Apr 15, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants